-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address review comments #1
Address review comments #1
Conversation
Hi @michaelbprice, because security is important I am trying to revive your package-url#245 by addressing some open review comments. I see you didn't yet respond to my comments. I hope you have some time to review this PR and if it is OK to you, merge it, so that your PR can be moved forward. |
Revive package-url#245 by adding the following changes: - `registry_url` -> `repository_url` - `registry_version` -> `port_revision` - Remove percent escaping from `repository_url` to be more consistent with other uses of `repository_url` in the purl spec
779c07c
to
06546ad
Compare
Currently very busy with CppCon. I'll take a look at your comment in my PR and this one that will update it when I get a chance. Feel free to ping me on it next week if I don't get to it by this weekend. |
Do take the time to enjoy CppCon! |
@michaelbprice Ping! 😄 |
@michaelbprice Ping 2! |
I'm still not convinced that percent-encoding the registry URL is "wrong", but I can live with removing that language. As was pointed out on the PR into the upstream, percent encoding could theoretically still be done, and parsers would need to deal with it. The situations where it would be absolutely needed (a registry URL that has qualifiers itself separated by ampersands) are unlikely to occur. |
I'd still like to retain the Without that snapshot in time, I'm worried that accidents will happen that will cause confusion or even that a malicious user could wreak havoc in some way that I haven't yet foreseen. But with that commit SHA there, if something has changed, at least someone investigating and tracing backwards could see that something has gone wrong. |
Co-authored-by: Michael B. Price <[email protected]>
Add optional `repository_revision` so that mistakes in `port_revision` and `version` can be accounted for. Not relevant for filesystem registries or overlay ports because that gives no further external traceability. Along with this, describe the filesystem registries and overlay port cases.
Extend the port overlay or filesystem registry example with port revision. Add an example for additional qualifiers.
Good point, I have added a I saw I also did not describe the filesystem registry or overlay port concept yet, so added that back. Also added a full example with additional, unspecified qualifiers. |
LGTM. Let's go! Hopefully this revives the upstream PR. They've got quite a backlog though. |
Revive package-url#245 by adding the following changes:
registry_url
->repository_url
registry_version
->port_revision
repository_url
to be more consistent with other uses ofrepository_url
in the purl spec